Skip to content

Comments

Add PagerDutyIntegration CRD#90

Merged
openshift-merge-robot merged 14 commits intoopenshift:masterfrom
grdryn:PagerDutyIntegrationCRD
Jul 20, 2020
Merged

Add PagerDutyIntegration CRD#90
openshift-merge-robot merged 14 commits intoopenshift:masterfrom
grdryn:PagerDutyIntegrationCRD

Conversation

@grdryn
Copy link
Member

@grdryn grdryn commented Jun 2, 2020

This change adds a PagerDutyIntegration CRD & replaces the
ClusterDeployment and SyncSet controllers with a single
PagerDutyIntegration controller. This will allow more than one
PagerDuty service to be created per cluster, so that more than one SRE
team can receive alerts for different concerns (e.g. layered products
on top of OSD).

Watches

Since there's only one controller, and there's a many-to-many
relationship between PagerDutyIntegrations and ClusterDeployments,
here's how reconciliation will be triggered:

  • Create/Update/Delete of a PagerDutyIntegration will trigger a
    reconcile request for that PagerDutyIntegration.
  • Create/Update/Delete of a ClusterDeployment will trigger a reconcile
    request for each PagerDutyIntegration that matches that
    ClusterDeployment (using the spec.clusterDeploymentSelector label
    selector).
  • Create/Update/Delete of a ConfigMap/Secret/SyncSet owned by a
    ClusterDeployment will trigger a reconcile request for each
    PagerDutyIntegration that matches that ClusterDeployment.

Migration / upgrade logic

This change also includes temporary migration logic (which can
optionally be removed easily in a subsequent release) to decide based
on an annotation whether the CR should be used for migration of
resources from the pre-PagerDutyIntegration CRD way of working. This
pd.openshift.io/legacy annotation will inform the operator that
there may be clusters that were configured with a pagerduty service
already, and that it should update the names of those resources to the
new format. Once all of the clusters have been migrated, it will
remove the annotation.

@grdryn grdryn force-pushed the PagerDutyIntegrationCRD branch 2 times, most recently from 350efb0 to cc03b66 Compare June 5, 2020 11:00
@jewzaam
Copy link
Member

jewzaam commented Jun 5, 2020

PR check needs a fix: #97

@grdryn grdryn force-pushed the PagerDutyIntegrationCRD branch from cc03b66 to 0fa18ef Compare June 8, 2020 14:57
@grdryn grdryn force-pushed the PagerDutyIntegrationCRD branch from 0fa18ef to 1ee1d5e Compare June 15, 2020 18:22
@grdryn
Copy link
Member Author

grdryn commented Jun 23, 2020

/cc @georgettica

@RiRa12621
Copy link

/assign @RiRa12621

@RiRa12621 RiRa12621 added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 30, 2020
@RiRa12621
Copy link

/hold
For time to sync on a plan going forward

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2020
Copy link

@georgettica georgettica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know what you added me but I tried to give this a pass. maybe some of the stuff will help

@RiRa12621
Copy link

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2020
Copy link

@RiRa12621 RiRa12621 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sane
see the comments + what @georgettica said

@grdryn
Copy link
Member Author

grdryn commented Jul 2, 2020

@RiRa12621 @georgettica Thanks a lot for the reviews! 🙂

I believe I've addressed all of the comments in one way or another, if you'd like to take a look again.

Copy link

@RiRa12621 RiRa12621 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2020
@RiRa12621
Copy link

/assign @jewzaam for approval

@openshift-ci-robot
Copy link

@RiRa12621: GitHub didn't allow me to assign the following users: for, approval.

Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @jewzaam for approval

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@grdryn
Copy link
Member Author

grdryn commented Jul 3, 2020

@RiRa12621 @jewzaam Thanks for the sign-off! :D Note that someone/something will need to create a PagerDutyIntegration CR for the details that were previously used in the old flow, and that CR should have the pd.openshift.io/legacy annotation to lift the existing resources for existing clusters into the new CR-based flow.

@RiRa12621
Copy link

it's gonna go to int and stage anyways first, so I'd assume we notice all kinds of "problems" like this before it hits prod

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2020
grdryn added 14 commits July 15, 2020 11:58
This is just the default generated types and CRD. A future commit will
flesh them out with the properties that we need in this project.
Command used:
operator-sdk add controller \
 --kind=PagerDutyIntegration \
 --api-version=pagerduty.openshift.io/v1alpha1
This doesn't yet watch everything, and it doesn't handle migration of
existing configured clusters to the new way with the
PagerDutyIntegration CRD.
Since a reconciliation request for this controller is the name and
namespace of a PagerDutyIntegration instance, and there might be
multiple instances tied to e.g. a ClusterDeployment instance on which
an event occurs; we want to trigger a reconciliation request for each
PagerDutyIntegration that may be affected.
Before this change there were several places where the names of
SyncSet/ConfigMap/Secret resources were decided, and since those are
more dynamic in this patch set, it makes sense to try to tidy it up a
bit.
Since this patch set changes the way this operator works to reconcile
a new custom resource, we need to be able to get existing
ClusterDeployments with their PagerDuty services into this new
format.

Rather than adding some sort of additional field to the spec that
would only ever be useful once for one CR, this looks for the
following annotation on CRDs: "pd.openshift.io/legacy" with a
non-empty value. This allows us to set this annotation on the OSD
PagerDutyIntegration CR, so that the operator knows that this is the
one to use for migration.

Once it has migrated all of the resources, it will delete the
annotation from the PagerDutyIntegration CR, so that it won't attempt
to migrate on every reconciliation.
This takes the old tests from the ClusterDeployment controller that
was here before, and makes them work with the PagerDutyIntegration
controller.

NOTE: Some of the tests have been commented out until some
dependencies get updated, as the fake k8s client can't handle list
options to a client.List call correctly.
@grdryn grdryn force-pushed the PagerDutyIntegrationCRD branch from a2c5f09 to 20f2b76 Compare July 15, 2020 11:08
@grdryn
Copy link
Member Author

grdryn commented Jul 15, 2020

This has been rebased now, as the changes to upgrade the operator-sdk version have been merged to master in #100

Copy link

@RiRa12621 RiRa12621 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/assign @jharrington22 for approval since @jewzaam is on PTO

@openshift-ci-robot
Copy link

@RiRa12621: GitHub didn't allow me to assign the following users: since, is, on, PTO, for, approval.

Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/lgtm
/assign @jharrington22 for approval since @jewzaam is on PTO

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2020
@jewzaam
Copy link
Member

jewzaam commented Jul 20, 2020

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn, jewzaam, RiRa12621

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2020
@openshift-merge-robot openshift-merge-robot merged commit 84a3b77 into openshift:master Jul 20, 2020
jewzaam added a commit to jewzaam/pagerduty-operator that referenced this pull request Jul 20, 2020
…ionCRD"

This reverts commit 84a3b77, reversing
changes made to 291012e.
jewzaam added a commit to jewzaam/pagerduty-operator that referenced this pull request Jul 20, 2020
…ionCRD"

This reverts commit 84a3b77, reversing
changes made to 291012e.

PagerDutyIntegration CRD is not deployed with the operator.  Operator cannot start.
openshift-merge-robot added a commit that referenced this pull request Jul 20, 2020
Revert "Merge pull request #90 from grdryn/PagerDutyIntegrationCRD"
@grdryn grdryn deleted the PagerDutyIntegrationCRD branch July 21, 2020 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants